[feature] Support generating pydantic types with Annotated attributes #9445
Conversation
aryehklein
had a problem deploying
to
Fern Dev
GitHub Actions
Failure
aryehklein
had a problem deploying
to
Fern Dev
GitHub Actions
Failure
|
|
||
| use_pydantic_field_aliases: bool = True | ||
|
|
||
| use_annotated_field_aliases: bool = False |
There was a problem hiding this comment.
High level points:
- If this looks good, I assume I also need a docs change - can you point me to where to make that?
- I added unit tests for this change, but didnt know how/where to do an end to end test with a full on generation?
There was a problem hiding this comment.
Great questions!
- We document the pydantic config in a separate repo specifically for our docs. You can see an analogous feature flag docs PR here: Document lazy_imports (Python SDK) docs#702. We're happy to make that PR though if you'd prefer! You've already been so generous with your time on this contribution.
- Our strategy for most ETE testing these days uses our framework called seed, which is a regression testing framework that you can read more about in CONTRIBUTING.md.
- For testing this, I'd probably recommend adding a new alias-extends fixture (could also use exhaustive) with this custom config set
- You'd then generate and check in the seed files
pnpm seed:local test --generator python-sdk --fixture alias-extends --outputFolder <whatever_you_name_the_output_folder>. This will also run ruff and mypy against the files - If you want more test coverage against the SDK, you can check in additional test files to the generated directory and .fernignore them
aryehklein
had a problem deploying
to
Fern Dev
GitHub Actions
Failure
aryehklein
had a problem deploying
to
Fern Dev
GitHub Actions
Failure
| # Use only the default value as initializer, not pydantic.Field | ||
| initializer = default_value | ||
|
|
||
| elif is_aliased and not self._use_pydantic_field_aliases: |
There was a problem hiding this comment.
There is a bit of complexity here with how this new feature interplays with use_pydantic_field_aliases. If use_pydantic_field_aliases is false, and _use_annotated_field_aliases is true, you get into double annotated types. The FastAPI generator defaults this to true, whereas the use_pydantic_field_aliases defaults to false. I think this if/elif block handles it, but lmk if there are other edge cases to consider!
There was a problem hiding this comment.
As far as I know this is reasonable! I'm good with the approach of just preferring it over the other flag (rather than e.g. breaking the config shape), as long as we document that that's the behavior.
| datetime: typing.Optional[dt.datetime] = None | ||
| date: typing.Optional[dt.date] = None | ||
| uuid_: typing_extensions.Annotated[typing.Optional[uuid.UUID], pydantic.Field(alias="uuid")] = None | ||
| base_64: typing_extensions.Annotated[typing.Optional[str], pydantic.Field(alias="base64")] = None |
There was a problem hiding this comment.
@tjb9dc This is a key line to look at to understand the diff. This line differs from the existing ones (e.g. https://github.com/fern-api/fern/blob/main/seed/fastapi/exhaustive/pydantic-v2/resources/types/resources/object/types/object_with_optional_field.py#L24) and is the core thing I want to solve.
| # Use only the default value as initializer, not pydantic.Field | ||
| initializer = default_value | ||
|
|
||
| elif is_aliased and not self._use_pydantic_field_aliases: |
There was a problem hiding this comment.
As far as I know this is reasonable! I'm good with the approach of just preferring it over the other flag (rather than e.g. breaking the config shape), as long as we document that that's the behavior.
| def _get_alias_from_type(type_: typing.Any) -> typing.Optional[str]: | ||
| maybe_annotated_type = _get_annotation(type_) | ||
|
|
||
| if maybe_annotated_type is not None: | ||
| # The actual annotations are 1 onward, the first is the annotated type | ||
| annotations = typing_extensions.get_args(maybe_annotated_type)[1:] | ||
|
|
||
| for annotation in annotations: | ||
| if isinstance(annotation, FieldMetadata) and annotation.alias is not None: | ||
| return annotation.alias | ||
| return None |
There was a problem hiding this comment.
I do wonder how/if this code will interact with this change. It's called (transitively) from the pydantic_utilities files, and we use different ones depending on the aliasing mode we're in. That decision (also factors in v1 vs v2) lives in src/fern_python/generators/core_utilities/core_utilities.py.
I think it's worth testing serializing some of your test models and making sure things look expected. From reading the code I think it's possible that we'll need to use the aliased pydantic utilities, which would mean updating the conditional statement in the three core_utilities.py files to provide that version if using the new feature flag you're adding. But definitely easier to figure that out by testing :)
|
This PR is stale because it has been open 25 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
github-actions
bot
added
the
Stale
|
This PR was closed because it has been inactive for 5 days after being marked stale. |
2 participants
Description
This PR comes to solve #9366.
Changes Made
Testing